Repository navigation
Conversation
microshine
left a comment
There was a problem hiding this comment.
Please revise the JSON typing approach before merging. I would like to keep this update compatible with the existing API and provide a complete typed representation.
There are two issues with the current change:
string | RelativeDistinguishedNamesJsondoes not describe alltoJSON()results. For example,iPAddressreturns ASN.1 JSON withouttypesAndValues. The narrowing used by the new consumer test (typeof json.value !== "string") therefore does not identify a directory name; the proposed type would allow readingjson.value.typesAndValues.lengthfor an IP address, which would fail at runtime. The other ASN.1 object variants must also be covered.- Widening the existing
valuetype breaks TypeScript compatibility, even for consumers of string variants. For example, this previously valid assignment stops compiling although its runtime value is still a string:
const email: string = new GeneralName({
type: 1,
value: "alice@example.com"
}).toJSON().value;Please consider extending the existing result with an optional typedValue field instead, preserving the current runtime values and declarations of type and value:
interface GeneralNameJson {
type: number;
value: string; // Keep the legacy declaration for compatibility.
typedValue?: GeneralNameTypedValue;
}The optional field preserves compatibility with existing object literals, stored JSON from earlier versions, and implementations that already return GeneralNameJson:
const json: GeneralNameJson = {
type: 2,
value: "example.com"
};For a valid DNS name, the extended result would be:
{
"type": 2,
"value": "example.com",
"typedValue": {
"kind": "dNSName",
"value": "example.com"
}
}GeneralNameTypedValue should be a complete discriminated union with its own kind field. These are suggested shapes for the new typedValue field only; retain the full existing value in each case:
| Type | Suggested typedValue |
|---|---|
| 0 — otherName | {"kind":"otherName","oid":"1.2.3.4","valueBerHex":"0c03666f6f"} |
| 1 — rfc822Name | {"kind":"rfc822Name","value":"alice@example.com"} |
| 2 — dNSName | {"kind":"dNSName","value":"example.com"} |
| 3 — x400Address | {"kind":"x400Address","berHex":"a3023000"} |
| 4 — directoryName | {"kind":"directoryName","attributes":[{"oid":"2.5.4.3","asn1Type":"UTF8String","value":"Example"}]} |
| 5 — ediPartyName | {"kind":"ediPartyName","nameAssigner":"Example","partyName":"Alice"} |
| 6 — uniformResourceIdentifier | {"kind":"uniformResourceIdentifier","value":"https://example.com"} |
| 7 — iPAddress | {"kind":"iPAddress","address":"127.0.0.1","bytesHex":"7f000001"} |
| 8 — registeredID | {"kind":"registeredID","value":"1.2.3.4"} |
A few details to account for:
otherNamepayloads depend on the OID, so the general representation should retain the BER of the payload rather than assume a particular decoded shape.- For
x400Address, retaining the BER of the original GeneralName ASN.1 node is sufficient; a complete X.400 object model need not be introduced here. - The directory-name attributes are a convenient projection; retain their OIDs and ASN.1 types, and keep the full legacy representation in
value. nameAssigneris optional forediPartyName. Please also check the schema-parsing path: it currently readsasn1.result.ediPartyName, but that name is not assigned in the schema. Do not produce a typed representation from missing data.- For IP addresses, cover IPv4, IPv6, and the address-and-mask form used in NameConstraints, retaining the original bytes.
Consumers could then use:
const typed = name.toJSON().typedValue;
if (typed?.kind === "directoryName") {
console.log(typed.attributes);
}
if (typed?.kind === "iPAddress") {
console.log(typed.address);
}If a correct typed representation cannot be produced, omit typedValue and preserve the existing behavior of toJSON() rather than introduce new exceptions.
This can be treated as a minor addition if the JSON contract permits additional fields. The output of JSON.stringify() will change, so consumers requiring an exact JSON shape need to be considered. Correcting the existing, imprecise value declaration can be reserved for a future major release.
…ring Keep GeneralNameJson.value declared as string so existing consumers still compile, and add an optional typedValue discriminated union keyed by kind covering all nine GeneralName types. typedValue is omitted when a faithful representation cannot be produced. Also parse ediPartyName from the matched [5] node: the schema never assigned asn1.result.ediPartyName, so parsed ediPartyName values were undefined.
|
Thanks for the detailed review. Reworked along your suggestion in 3ed8d4d:
|
# Conflicts: # src/GeneralName.ts
Master now only collects src/**/*.spec.ts. Also formats both files with oxfmt and renames a local that shadowed the hex import.
Summary
GeneralName.toJSON()now adds an optionaltypedValuenext tovalue.valuekeeps its current type and output, so existing callers do not break.typedValueis a union keyed bykindthat covers all nine CHOICE arms: otherName (OID plus BER hex), rfc822Name, dNSName, x400Address (BER hex), directoryName (attributes), ediPartyName, uniformResourceIdentifier, iPAddress (address, optional mask and raw bytes) and registeredID. It is left out when a name cannot be represented.This follows the review asking for a compatible change with a complete typed form. The tradeoff is that
JSON.stringifyoutput gains a field.It also fixes ediPartyName parsing, which now reads the matched
[5]node.Fixes #495
Tests
src/GeneralName.spec.ts(npx vitest run src/GeneralName.spec.ts) covers each kind, an otherName built by hand as an OtherName SEQUENCE, ediPartyName with and withoutnameAssigner, IPv4 and IPv6 with and without a mask, the omitted case, and a TypeScript consumer narrowing onkind. 8 of these 9 tests fail withsrc/GeneralName.tsreverted to master. The spec also keeps the two otherName encoding tests from #586.